Load proposals sequentially #1506

adamgall · 2024-03-30T20:41:09Z

Closes #1509
Closes #1457
Closes #1464

User facing changes

Loads Azorius proposals sequentially instead of all at once.
No change to how Multisig or Snapshot proposals / transactions are loaded.

Behind the scenes changes

Moved a handful of event listeners from various places (mostly useAzoriusProposals, but also useERC20LinearStrategy and useERC721LinearStrategy) into their own new top-level hook, useAzoriusListeners
Optimized proposal object creation logic to no longer need to fetch Voted events or ProposalExecuted events for each proposal. Instead, fetch them all just once before processing each proposal. Because they're not indexed, we have to just get all of them anyway. So just pass them down into the proposal processing and filter the same static set per proposal.
Plenty of other stuff, but I guess mostly just a lot of refactoring.
- Start with useAzoriusProposals, then check out azorius.ts, then check out useAzoriusListeners, then check out the other little things strewn around.

Potential follow up work

More consistent and accurate "loading" components for the proposals. Both when they start (still seeing "no proposals" message before the first one loads in), and while they're loading (would be good to have a "more proposals loading" component at the bottom that stays there until they're all done).
Cache completed proposals in local storage, so we don't need to re-fetch them.

Testing

Bring up a DAO with many Azorius proposals: see how they load.
Before they're finished loading in, switch to a new DAO (in-app, via Favorites or Hierarchy or Search): see how the first DAO's proposals don't populate into this new DAO yay.
All other user experience and behavior should be unchanged.

…nto state

… about an address just make a single request, don't requestWithRetries

netlify · 2024-03-30T20:41:28Z

✅ Deploy Preview for fractal-dev ready!

Name	Link
🔨 Latest commit	`db8823b`
🔍 Latest deploy log	https://app.netlify.com/sites/fractal-dev/deploys/660dca99bb9993000869e3bb
😎 Deploy Preview	https://deploy-preview-1506.app.dev.fractalframework.xyz
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Currently we're truncating Markdown in Proposal List card, which is a giant link. Not supposed to render anchor tags within anchor tags.

… file, extensive use of dependency injection

…trategy contracts

Inline the `loadAzoriusProposals` again so we can use a ref to track the current DAO and stop the sequential processing of proposals if the DAO changes.

…h up

adamgall · 2024-04-03T21:04:23Z

package.json

@@ -92,7 +92,6 @@
    "@testing-library/react": "^14.2.1",
    "@types/react": "^18.0.17",
    "@types/react-dom": "^18.0.6",
-    "@types/remove-markdown": "^0.3.4",


Noticed that this unused package was hanging around, so removed it.

adamgall · 2024-04-03T21:05:25Z

src/components/ui/proposal/Markdown.tsx

@@ -89,7 +89,7 @@ export default function Markdown({ truncate, content, collapsedLines = 6 }: IMar
        maxWidth="100%"
      >
        <ReactMarkdown
-          remarkPlugins={[remarkGfm]}
+          remarkPlugins={truncate ? [] : [remarkGfm]}


Don't try to render URLs as actual links if we're showing markdown in "truncate" mode.

We're using "truncate" when showing markdown in Proposal List cards, which are giant anchor tags, so we don't want to try to render more anchor tags within those.

this doesn't have anything to do with the rest of this PR

adamgall · 2024-04-03T21:07:12Z

src/hooks/DAO/loaders/governance/useERC20LinearStrategy.ts

In this file, removed a bunch of references to governance type from the dependency arrays of useEffects, which weren't used in the actual hooks.

@mudrila @Da-Colon do you know why these were here? Are they necessary?

I added this bandaid, safe to remove. This was to try to make it wait a little longer before it tries to setup the listener. which is no longer needed as this work fixes the root problem

Most likely some leftovers, seems like those references are not needed here

adamgall · 2024-04-03T21:18:07Z

src/hooks/DAO/loaders/governance/useERC20LinearStrategy.ts

-  useEffect(() => {
-    if (!azoriusContractAddress || !baseContracts || !type) {
-      return;
-    }
-    const azoriusContract =
-      baseContracts.fractalAzoriusMasterCopyContract.asProvider.attach(azoriusContractAddress);
-    const timeLockPeriodFilter = azoriusContract.filters.TimelockPeriodUpdated();
-    const timelockPeriodListener: TypedListener<TimelockPeriodUpdatedEvent> = timelockPeriod => {
-      action.dispatch({
-        type: FractalGovernanceAction.UPDATE_TIMELOCK_PERIOD,
-        payload: BigNumber.from(timelockPeriod),
-      });
-    };
-    azoriusContract.on(timeLockPeriodFilter, timelockPeriodListener);
-    return () => {
-      azoriusContract.off(timeLockPeriodFilter, timelockPeriodListener);
-    };
-  }, [azoriusContractAddress, action, baseContracts, type]);


Moved this event listener to new useAzoriusListeners

adamgall · 2024-04-03T21:23:32Z

src/hooks/DAO/loaders/governance/useERC721LinearStrategy.ts

-  useEffect(() => {
-    if (!azoriusContractAddress || !baseContracts || !type) {
-      return;
-    }
-    const azoriusContract =
-      baseContracts.fractalAzoriusMasterCopyContract.asProvider.attach(azoriusContractAddress);
-
-    const timeLockPeriodFilter = azoriusContract.filters.TimelockPeriodUpdated();
-    const timelockPeriodListener: TypedListener<TimelockPeriodUpdatedEvent> = timelockPeriod => {
-      action.dispatch({
-        type: FractalGovernanceAction.UPDATE_TIMELOCK_PERIOD,
-        payload: BigNumber.from(timelockPeriod),
-      });
-    };
-    azoriusContract.on(timeLockPeriodFilter, timelockPeriodListener);
-    return () => {
-      azoriusContract.off(timeLockPeriodFilter, timelockPeriodListener);
-    };
-  }, [azoriusContractAddress, action, baseContracts, type]);


This was copied line-for-line in useERC20LinearStrategy.ts, so removed here. I removed it from the other file too, and it's now in useAzoriusListeners.

adamgall · 2024-04-03T21:27:02Z

src/hooks/DAO/useDAOController.ts

@@ -48,6 +49,7 @@ export default function useDAOController() {
  useFractalTreasury();
  useERC20Claim();
  useSnapshotProposals();
+  useAzoriusListeners();


Create the listeners.

adamgall · 2024-04-03T21:27:18Z

src/providers/App/governance/action.ts

+  | {
+      type: FractalGovernanceAction.SET_AZORIUS_PROPOSAL;
+      payload: FractalProposal;
+    }


A new action for setting one single Azorius proposal into state

adamgall · 2024-04-03T21:27:43Z

src/providers/NetworkConfig/web3-modal.config.ts

+      {
+        batch: true,
+      },


Can't hurt right?

not at all. I think we will see these benefits when we conform more to the Wagmi patterns and hooks ie useContractsRead

adamgall · 2024-04-03T21:28:51Z

src/utils/azorius.ts

-    const proposalExecutedFilter = azoriusContract.filters.ProposalExecuted();
-    const proposalExecutedEvents = await azoriusContract.queryFilter(proposalExecutedFilter);


We don't need to re-fetch these events for each proposal, can just do it once way up above and pass the full array down here for filtering.

adamgall · 2024-04-03T21:29:58Z

src/utils/azorius.ts

Lots of refactoring in here to deal with teasing out the slight differences between ERC20 strategy contract and ERC721 strategy contract.

tomstuart123 · 2024-04-04T17:10:17Z

This works really well for me. Load time of 1 (rather than multiple) proposal(s) feels quicker and they all come in in the right order (from new to late)

I raised at standup my desire for more comms on load time but @adamgall highlighted he raised these above. This led me to create the card here - #1517

mudrila

🚢

Da-Colon

Overall, gonna approved. just some questions

Da-Colon · 2024-04-05T14:00:14Z

src/hooks/DAO/loaders/governance/useERC20LinearStrategy.ts

I added this bandaid, safe to remove. This was to try to make it wait a little longer before it tries to setup the listener. which is no longer needed as this work fixes the root problem

Da-Colon · 2024-04-05T14:00:50Z

src/hooks/DAO/loaders/governance/useERC721LinearStrategy.ts

I added this bandaid, safe to remove. This was to try to make it wait a little longer before it tries to setup the listener. which is no longer needed as this work fixes the root problem

Da-Colon · 2024-04-05T15:02:01Z

src/hooks/DAO/loaders/useFractalNode.ts

-          () => safeAPI.getSafeInfo(utils.getAddress(_daoAddress)),
-          5,
-        );
+        safeInfo = await safeAPI.getSafeInfo(utils.getAddress(_daoAddress));


This has been here for a long time. This was to safe guard against network hiccups. But yeah generally probably safe to remove

Da-Colon · 2024-04-05T15:14:29Z

src/providers/NetworkConfig/web3-modal.config.ts

+      {
+        batch: true,
+      },


not at all. I think we will see these benefits when we conform more to the Wagmi patterns and hooks ie useContractsRead

src/utils/azorius.ts

Da-Colon · 2024-04-05T15:17:21Z

src/utils/azorius.ts

+export const decodeTransactions = async (
+  _decode: (value: string, to: string, data?: string | undefined) => Promise<DecodedTransaction[]>,
+  _transactions: MetaTransaction[],
+) => {
+  const decodedTransactions = await Promise.all(
+    _transactions.map(async tx => _decode(tx.value.toString(), tx.to, tx.data)),
+  );
+  return decodedTransactions.flat();
+};


Where did this appear from?

It came from here: https://github.com/decentdao/fractal-interface/pull/1506/files/db8823bfe3abb068126f25dac7174601deedfaf2#diff-051684bdee1b274984af1420c9548bf363cd143b226a8d3f2692daddf0148afbL40-L50

I just moved it into azorius.ts because there was another new file (useAzoriusListeners.ts) that needed it.

adamgall added 4 commits March 30, 2024 16:31

Set "batch" requesting, can't hurt right lol

d711335

Make a new action and reducer for setting a single Azorius proposal i…

951f587

…nto state

Unrelated to the rest of the work in here, but when getting Safe info…

ccf7b60

… about an address just make a single request, don't requestWithRetries

Comment out all of the Azorius Proposal Listeners

3f549b6

Load proposals synchronously

ab359c0

adamgall force-pushed the load-proposals-synchronously branch from d374c2f to ab359c0 Compare March 30, 2024 20:45

adamgall added 4 commits March 30, 2024 20:14

Get all Voted and ProposalExecuted events one time, not per proposal

5409650

Rename function and do less param passing

8fe9d1b

Remove the @types/remove-markdown package, not used

c7a6359

If truncating Markdown, don't try to render URLs

bec53f2

Currently we're truncating Markdown in Proposal List card, which is a giant link. Not supposed to render anchor tags within anchor tags.

adamgall force-pushed the load-proposals-synchronously branch from 2a2794a to bec53f2 Compare March 31, 2024 00:15

adamgall added 3 commits March 31, 2024 15:52

Same event listener set up twice

cd0be94

Hm, not sure why these values are part of useeffect arrays

d3f3999

Broke out a lot of functions from useAzoriusProposals to top level of…

bf6af3f

… file, extensive use of dependency injection

adamgall changed the title ~~WIP: Load proposals synchronously~~ WIP: Load proposals sequentially Apr 1, 2024

adamgall added 11 commits April 2, 2024 16:19

Remove fully reloading azorius proposals after creating one

c741273

Big refactor to re-enable setting up event listeners on Azorius and S…

f813b30

…trategy contracts

Remove unneeded dependency

9502942

Remove catch error, let's see if it keeps happening

cd96779

Move proposal creation / voted event listeners to own hook

f9f8c34

Remove dev testing console.logs

8a05370

Remove unnecessary function

7c45797

Remove another unneeded object

fa26f01

Code is a flat circle

02adb1a

Inline the `loadAzoriusProposals` again so we can use a ref to track the current DAO and stop the sequential processing of proposals if the DAO changes.

Remove stray console.log

7d50962

Delay processing new proposal for a block, to give RPC chance to catc…

72f74ab

…h up

adamgall marked this pull request as ready for review April 3, 2024 20:48

adamgall requested review from mudrila and tomstuart123 April 3, 2024 20:48

adamgall requested a review from Da-Colon April 3, 2024 20:48

adamgall changed the title ~~WIP: Load proposals sequentially~~ Load proposals sequentially Apr 3, 2024

adamgall added 2 commits April 3, 2024 17:16

Lint fixes, move listener

8df3b39

More unnecessary dependency array params

d1db677

adamgall commented Apr 3, 2024

View reviewed changes

lint fixes

db8823b

adamgall requested a review from DarksightKellar April 3, 2024 21:41

adamgall self-assigned this Apr 4, 2024

tomstuart123 mentioned this pull request Apr 4, 2024

Sequential Proposal follow up -Caching #1517

Closed

tomstuart123 approved these changes Apr 4, 2024

View reviewed changes

mudrila approved these changes Apr 5, 2024

View reviewed changes

Da-Colon reviewed Apr 5, 2024

View reviewed changes

Da-Colon approved these changes Apr 5, 2024

View reviewed changes

adamgall merged commit 20b4ba3 into develop Apr 5, 2024
7 checks passed

adamgall deleted the load-proposals-synchronously branch April 5, 2024 16:24

adamgall mentioned this pull request Apr 5, 2024

Release/2024-04-08 Part 1: The Merge into prod #1521

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load proposals sequentially #1506

Load proposals sequentially #1506

adamgall commented Mar 30, 2024 •

edited

Loading

netlify bot commented Mar 30, 2024 •

edited

Loading

adamgall Apr 3, 2024

adamgall Apr 3, 2024

adamgall Apr 4, 2024

adamgall Apr 3, 2024

Da-Colon Apr 5, 2024

mudrila Apr 5, 2024

adamgall Apr 3, 2024

adamgall Apr 3, 2024

adamgall Apr 3, 2024

adamgall Apr 3, 2024

adamgall Apr 3, 2024

Da-Colon Apr 5, 2024

adamgall Apr 3, 2024

adamgall Apr 3, 2024

tomstuart123 commented Apr 4, 2024

mudrila left a comment

Da-Colon left a comment

Da-Colon Apr 5, 2024

Da-Colon Apr 5, 2024

Da-Colon Apr 5, 2024

Da-Colon Apr 5, 2024

Da-Colon Apr 5, 2024

adamgall Apr 5, 2024

		const proposalExecutedFilter = azoriusContract.filters.ProposalExecuted();
		const proposalExecutedEvents = await azoriusContract.queryFilter(proposalExecutedFilter);

Load proposals sequentially #1506

Load proposals sequentially #1506

Conversation

adamgall commented Mar 30, 2024 • edited Loading

User facing changes

Behind the scenes changes

Potential follow up work

Testing

netlify bot commented Mar 30, 2024 • edited Loading

✅ Deploy Preview for fractal-dev ready!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomstuart123 commented Apr 4, 2024

mudrila left a comment

Choose a reason for hiding this comment

Da-Colon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamgall commented Mar 30, 2024 •

edited

Loading

netlify bot commented Mar 30, 2024 •

edited

Loading